[fix](table stream) force VCollectIterator merge in case of binlog scan - #66338
Open
TsukiokaKogane wants to merge 5 commits into
Open
[fix](table stream) force VCollectIterator merge in case of binlog scan #66338TsukiokaKogane wants to merge 5 commits into
TsukiokaKogane wants to merge 5 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
TsukiokaKogane
changed the title
TsukiokaKogane
marked this pull request as draft
July 31, 2026 11:15
Contributor
TPC-H: Total hot run time: 29218 ms |
Contributor
TPC-DS: Total hot run time: 169216 ms |
Contributor
ClickBench: Total hot run time: 23.94 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TsukiokaKogane
marked this pull request as ready for review
August 4, 2026 08:16
Contributor
Author
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Two issues need resolution; see the inline comments.
Critical checkpoint conclusions:
- Goal and proof: The APPEND_ONLY crash trigger is correctly identified, and the DUP/MoW sections cover that direct block path. However, the blanket union-mode change leaves the supported MIN_DELTA/DETAIL row-wise child handoff able to crash (P1), so the fix is not complete across parallel consumers. The schema-derived
is_uniquechange is correct, but its distinct event-preservation behavior has no direct regression oracle (P2). - Scope and parallel paths: The production edits are localized. Ordinary/projected UNIQUE reads, AGG paths, row-binlog DUP semantics, compaction, schema-change, checksum, and cloud/manual uniqueness contexts were traced. No storage-format, protocol, or rolling-upgrade incompatibility was introduced.
- Concurrency, lifecycle, and errors: The iterator state is scanner-local and sequential; no new threads, locks, atomics, or lock-order concerns apply. The surviving defect is lifecycle initialization at union-child handoff: a later child can reach a null block before reader status propagation. Shared-block ownership, same bits, EOF handling, row locations, reverse traversal, and empty-child erasure are otherwise coherent once the child is initialized.
- Configuration and observability: No production configuration is added. The regression's temporary BE setting and debug point are scoped and restored, and the non-cloud restriction matches existing row-binlog support. No new metric or log is required for this local correction.
- Tests and results: The added output is internally consistent and deterministically ordered for the APPEND_ONLY cases. Coverage is missing for a row-wise multi-rowset handoff and for same-key/same-TSO events split across segments of one forced-merge rowset. No build or test was run during this static-only review.
- Persistence and writes: The production change is read-time only; it adds no transaction, persistence, edit-log, write-atomicity, or FE-BE variable work.
- Performance: No separate substantiated performance blocker survived review; correctness initialization must be fixed first.
- User focus: No additional user-provided focus was present, so the whole PR received the normal complete review.
Review completion: complete after two review rounds; all candidates were accepted, deduplicated, or dismissed before freezing this request-changes payload.
| // CHECK(!_get_data_by_ref). See issue #65901. | ||
| _is_merge_iterator = _rs_reader->is_merge_iterator(); | ||
| _get_data_by_ref = get_data_by_ref && _is_merge_iterator; | ||
| _get_data_by_ref = false; |
Contributor
There was a problem hiding this comment.
[P1] Initialize later block-mode union children before row-wise handoff
This forces every union child into block mode, but init_for_union() still does not allocate _block or initialize _ref for children after the first (only the first non-empty child reaches ensure_first_row_ref()). When a row-wise consumer such as MIN_DELTA/DETAIL advances to a later multi-segment child, next(IteratorRowRef*) takes the new false branch and evaluates _block->rows() before refresh_current_row() can lazily allocate it, so a pair of key-disjoint multi-segment rowsets can null-dereference at child handoff. Please establish a valid empty block/pre-first-row state for every child entering block mode (resetting cursors alone is insufficient), and cover a row-wise union consumer as well as APPEND_ONLY.
Contributor
Author
There was a problem hiding this comment.
looking into it
| // every row is an independent change event that must not be deduplicated by user | ||
| // key; using tablet()->keys_type() (UNIQUE_KEYS for MoW/UNIQUE tables) would make | ||
| // the rowset merge iterator drop events across segments. Mirrors VerticalBlockReader. | ||
| _reader_context.is_unique = _tablet_schema->keys_type() == UNIQUE_KEYS; |
Contributor
There was a problem hiding this comment.
[P2] Add coverage for equal keys across segments of one rowset
This flag is consumed by BetaRowsetReader only when its per-rowset VMergeIterator compares segments. Every new load here is internally key-unique; Section C overlaps two separate rowsets/Tsos, so the outer VCollectIterator handles it and the suite still passes if only this line is reverted. The older raw binlog() multi-segment test also normally uses scan type NONE and does not force this internal merge. Please add one table-stream load whose same key/TSO events cross a forced segment boundary and assert the retained event chain, so the event-loss behavior fixed by this line has a direct regression.
Contributor
TPC-H: Total hot run time: 28727 ms |
Contributor
TPC-DS: Total hot run time: 166289 ms |
Contributor
ClickBench: Total hot run time: 23.9 s |
TsukiokaKogane
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
HappenLee
reviewed
Aug 6, 2026
| @@ -60,9 +60,9 @@ class BetaRowsetReader : public RowsetReader { | |||
| } | |||
|
|
|||
| bool is_merge_iterator() const override { | |||
Contributor
There was a problem hiding this comment.
如果这里只有一个segment的话，这个force是不是没有必要
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #65901
Related PR: #xxx
Problem Summary:
Querying an APPEND_ONLY table stream backed by ROW binlog crashes BE with Check failed: !_get_data_by_ref when a data rowset holds multiple segments.
Reproduction: create a DUP or MoW table with ROW binlog enabled, create an APPEND_ONLY stream on it, load enough non-overlapping data so that a rowset is classified as NONOVERLAPPING but contains more than one segment, then SELECT
... FROM stream. It does not reproduce with single-segment rowsets.
Root cause: a table-stream scan forces
force_key_ordered_read = true, so a rowset with more than one segment makesBetaRowsetReader::is_merge_iterator()return true. Because the rowsets are NONOVERLAPPING, the outer VCollectIteratorpicks block-batch mode (_merge == false) and goes through the union path. However, Level0Iterator::init_for_union() still computed
_get_data_by_ref = get_data_by_ref && _is_merge_iterator, so a non-first child was put into by-reference mode. _get_data_by_ref is only meaningful on the merge path (where init() prefetches the block view and the child is consumed row by row through the merge heap); the union path always reads whole blocks via
next(Block*)/ensure_first_row_ref(), both of which assert !_get_data_by_ref. The two independently selected modes conflict and the assertion aborts BE.Fix: on the non-merge union path, Level0Iterator::init_for_union() now always sets _get_data_by_ref = false, keeping every child in block-batch mode consistent with the union path. The merge path (rowsets overlapping) goes through
init() and is unaffected.
Before: BE aborts with SIGABRT (Check failed: !_get_data_by_ref) at VCollectIterator::Level0Iterator::ensure_first_row_ref() / next(Block*).
After: the stream query completes and returns all rows correctly.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)